Skip to content

ARROW-6303: [Rust] Add a feature to disable SIMD#5269

Closed
paddyhoran wants to merge 12 commits intoapache:masterfrom
paddyhoran:simd-feature
Closed

ARROW-6303: [Rust] Add a feature to disable SIMD#5269
paddyhoran wants to merge 12 commits intoapache:masterfrom
paddyhoran:simd-feature

Conversation

@paddyhoran
Copy link
Contributor

@paddyhoran paddyhoran commented Sep 4, 2019

No description provided.

@paddyhoran paddyhoran marked this pull request as ready for review September 5, 2019 20:19
@paddyhoran
Copy link
Contributor Author

paddyhoran commented Sep 5, 2019

CI failure is unrelated. @andygrove @sunchao @nevi-me @liurenjie1024 PTAL

@paddyhoran paddyhoran mentioned this pull request Sep 7, 2019
Closed
nevi-me
nevi-me reviewed Sep 8, 2019
View reviewed changes
Copy link
Contributor

@nevi-me nevi-me left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 Question around build times, and I'm happy with the change

ci/rust-build-main.bat Outdated
Copy link
Contributor

@nevi-me nevi-me Sep 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that in one of the earlier PRs we reduced the amount that this script does (83a4e97#diff-09e63d5097178234f5b3b9d607d9c88aL47), won't we run the risk of increasing build times too much if we build 2 --release binaries here?

Copy link
Contributor Author

@paddyhoran paddyhoran Sep 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, maybe. I posted this before the recent issues with build times. I don't think that this will be used often but we should make sure to test that it at least builds. I'll change it to a debug build.

@paddyhoran
Copy link
Contributor Author

paddyhoran commented Sep 10, 2019

@hengruo @nevi-me it seems that I will need to revert ARROW-6408.

Although it would eventually be removed, using if cfg!(...) leaves the SIMD code in place for the first pass of the compiler leading to use of undeclared type or module error.

@nevi-me
Copy link
Contributor

nevi-me commented Sep 10, 2019

@hengruo @nevi-me it seems that I will need to revert ARROW-6408.

Although it would eventually be removed, using if cfg!(...) leaves the SIMD code in place for the first pass of the compiler leading to use of undeclared type or module error.

Sorry, only seeing this now. Do we still need to revert?

@paddyhoran
Copy link
Contributor Author

paddyhoran commented Sep 10, 2019

Do we still need to revert?

I think so yes, but I am having an issue with CI as well. I don't think that travis is triggering the --no-default-features build, it seems that this line is not executed. In the logs you can see that it only compiles once before running the tests, I was expecting it to compile twice.

I was hoping to look at the AppVeyor logs (I'm more familiar with windows) after the most recent commit but it failed before running the Rust build.

I can't build locally as it is now, but I reverted the changes last night and I can build it.

andygrove
andygrove reviewed Sep 10, 2019
View reviewed changes
Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM .. will approve once the build is passing

@paddyhoran
Copy link
Contributor Author

paddyhoran commented Sep 18, 2019

This was confusing me no end as I could not build locally without reverting 6408. I found that the --no-default-features flag is not passed from the workspace to the sub-crates. I'm going to update CI to prove it out and then fix it.

@codecov-io
Copy link

codecov-io commented Sep 18, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@4be02c7). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5269   +/-   ##
=========================================
  Coverage          ?   88.63%           
=========================================
  Files             ?      956           
  Lines             ?   126976           
  Branches          ?     1495           
=========================================
  Hits              ?   112540           
  Misses            ?    14071           
  Partials          ?      365

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4be02c7...5ab3125. Read the comment docs.

@paddyhoran
Copy link
Contributor Author

paddyhoran commented Sep 19, 2019

@andygrove @nevi-me I think this is ready to be merged now, quick review if you have time please.

andygrove
andygrove approved these changes Sep 22, 2019
View reviewed changes
Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @paddyhoran

@andygrove andygrove closed this in fec7143 Sep 22, 2019
@paddyhoran paddyhoran deleted the simd-feature branch February 25, 2020 02:31
@asfimport asfimport mentioned this pull request Sep 22, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nevi-me nevi-me nevi-me left review comments

@andygrove andygrove andygrove approved these changes

Assignees

No one assigned

Labels

Component: Rust

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@paddyhoran @nevi-me @codecov-io @andygrove